feat: lock token transfer and parameter module #3176
Conversation
github-actions
bot
added
🧾 package/realm
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Thank you for pushing this out 🙏
I think the direction is good, but I'd love to discuss more about some implementation details. I'm mostly worried about us having a temporary implementation detail as a permanent state of a core object (check comments).
Pinging @moul to give a review as well.
Please check the CI 🙏
| stdout '(0 uint64)' | ||
|
|
||
|
|
||
| ## vote unlock proposal with unrestricted account test1 |
There was a problem hiding this comment.
Which accounts are going to be unrestricted in the initial version of the chain?
There was a problem hiding this comment.
For now, all GovDAO accounts that need to vote must be unrestricted from token transfer locking, as voting on a proposal requires sending fees to the contract.
gno.land/pkg/gnoland/app_test.go
Outdated
| {key: "foo", kind: "bool", value: true}, | ||
| {key: "foo", kind: "bytes", value: []byte{0x48, 0x69, 0x21}}, | ||
| }, | ||
| /* |
There was a problem hiding this comment.
This is commented out since it conflicts with the alternative approach for the Params module in this PR. I believe we should avoid setting arbitrary parameter values in the genesis file without going through module validation.
We need to discuss it and agree on the approach.
| // Otherwise, we cannot verify the unrestricted address in the genesis state. | ||
|
|
||
| for _, addr := range data.Params.UnrestrictedAddrs { | ||
| acc := ak.GetAccount(ctx, addr) |
There was a problem hiding this comment.
There is tight coupling here. The account keeper must have a previously initialized account in order for us to modify it in another func
Can't we keep the store of unrestricted accounts somewhere, instead of keeping the Unrestricted information on the account itself?
My biggest concern is that we're coupling temporary logic (account balance transfer locks) into a structure that will be encoded and saved permanently to disk. Every time we use the account, even in 10 years, we will have to keep track of its "restricted state".
This is why I'm suggesting we go with an approach that isn't so coupled
There was a problem hiding this comment.
There are two reasons for implementing it this way:
-
There was a previous requirement to track and unlock token transfers on an individual account basis, not just for whitelisted accounts. That is the reason the restricted state is on the account level. We can revisit the requirement, otherwise, we will need to track the restricted state for a long time.
-
The whitelisted unrestricted accounts need to be validated to ensure they exist when we load the genesis state.
There was a problem hiding this comment.
into a structure that will be encoded and saved permanently to disk.
It is something that new chains will probably need for US regulatory purposes so it might be something we want to support long term.
Also, we can use a bitmask (uint64) instead of a bool, to save space, and we can have more flags.
- There was a previous requirement to track and unlock token transfers on an individual account basis
Is there more info on this?
- The whitelisted unrestricted accounts need to be validated to ensure they exist when we load the genesis state.
doesn't seem like a reason that necessitates this solution?
still thinking...
| ok, err := ak.paramk.GetParams(ctx, ModuleName, "p", params) | ||
|
|
||
| if !ok { | ||
| panic("params key " + ModuleName + " does not exist") |
There was a problem hiding this comment.
The module parameter keys are predefined and store in the Param module. If we try to access a predefined key that does not exist, the program should panic.
tm2/pkg/sdk/auth/params.go
Outdated
| TxSizeCostPerByte int64 `json:"tx_size_cost_per_byte" yaml:"tx_size_cost_per_byte"` | ||
| SigVerifyCostED25519 int64 `json:"sig_verify_cost_ed25519" yaml:"sig_verify_cost_ed25519"` | ||
| SigVerifyCostSecp256k1 int64 `json:"sig_verify_cost_secp256k1" yaml:"sig_verify_cost_secp256k1"` | ||
| UnrestrictedAddrs []crypto.Address `json:"unrestricted_addrs" yaml:"unrestricted_addrs"` |
There was a problem hiding this comment.
Just a note we don't initialize this new field in DefaultParams
There was a problem hiding this comment.
By default, there are no unrestricted accounts since token transfer locking is off. Unrestricted accounts should be added to the genesis file when token locking is set to true. These accounts can be added manually or through a separate genesis command later in the production deployment.
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
I was asked by Manfred to provide some feedback on the two different approaches. I don't have strong opinions one way or the other, here's what I consider:
If I was forced to choose, the "generic" approach seems to me simple to understand and to build on top of, and like I would shoot myself less frequently in the foot. But I'm not heavily swinging either way. |
|
@thehowl @moul The issue is not limited to key name and type validation. Allowing the creation and update of arbitrary chain parameters is unsafe and could lead to undetectable mistakes, potentially leaving the chain vulnerable to exploitation later. It's like using a map when we should be using a struct to pass parameters to the critical sections of a program. Here are some specifics:
In practice, proposals like this are difficult to verify in terms of their behavior and consequences because the behavior is implemented in the code itself.
Configurations can easily become out of sync with the code implementation without being noticed. This makes it challenging to track how these settings are linked in the code and how they impact chain behavior. This issue is especially problematic when we need to support backward-compatible features.
This constant does not match the genesis configuration, meaning changes in the genesis will never update the value of chainDomainParamPath as intended. gno/gno.land/genesis/genesis_params.toml Line 10 in 3fd5571 gno/gno.land/pkg/sdk/vm/params.go Line 7 in 3fd5571 It could get worse. Imagine if this property were created by GovDao instead of the genesis. All these small mistakes would be extremely difficult to detect. The old key could remain in the chain even after creating a new key |
| @@ -1,132 +0,0 @@ | |||
| package main | |||
There was a problem hiding this comment.
What do you think about introducing a filetest for the locking, since we dropped this string param one?
There was a problem hiding this comment.
The non-typed method of using r/sys/params module is and still should be supported, so this test shouldn't be deleted.
|
|
||
| func (bank BankKeeper) sendCoins( | ||
| ctx sdk.Context, | ||
| fromAddr crypto.Address, |
|
I'll try to sum up my primary concerns with this PR, and we just need to agree if we're okay with this at this point in the chain's lifecycle: Things that keep me worried about the implementation:
What I like about the PR:
In terms of which approach to go with, maybe we can opt for a fusion of the two ideas?
I'm more inclined to opt for the field approach Manfred initially advocated for, because it seems more durable and future-proof. We already support most field types, so I can't foresee an issue when we need to change values or update structs (completely revamp what "params" are). We also don't encounter the |
|
This PR supports both individual field parameters and struct types. The most important gap we aim to resolve between this PR and the existing params implementation is the following: a) The current implementation treats the Param module as a native store, allowing arbitrary chain parameter key-value pairs to be added from both genesis and GovDao. b) This PR predefines chain parameters in the code, allowing only their values to be modified from genesis and GovDao. |
| _, rlmPath := currentRealm(m) | ||
| GetContext(m).Params.SetCurRealmPath(rlmPath) |
There was a problem hiding this comment.
there's something strange about this side effect being made here. thinking...
There was a problem hiding this comment.
Intuitively, there's no reason why anything like SetCurRealmPath needs to be called here, because all the information is already encapsulated in the result value (the first part). Or said differently, one would expect from the method name and arguments that the function is side effect free. And while comments could help, the complexity isn't justified.
Immediately after calls to pkey() are calls to GetContext(m).Params.SetXYZ(), which then read curRealmPath (which was just set before). This is indirect and unnecessary and sort of works, but it's easy to intentionally or accidentally add vulnerabilities. For example, a developer might implement X_setParamClock which calls Params.SetUint64() without even calling pkey because the key is always supposed to be some global const. Or maybe among all the complex edge cases in Params.SetBool(), one might end up constructing a throwaway machine with the same Params and setting a param there, which would end up overriding curRealmPath with the realm path of the throwaway machine's realm.
X_setParamBool()
pkey(m1, ...)
GetContext(m).Params.SetBool()
pkey(m2, ...)
GetContext(m).Params.SetString()
Params.curRealmPath is expected to be m1's realm, but is actually m2's realm.
The above example is a bit convoluted because it's not clear why you'd ever want to instantiate m2 in the first place, but it's on a similar level of complexity being introduced in this PR.
The other use of SetCurRealmPath is in https://github.com/gnolang/gno/pull/3176/files#r1908550538 in a test, but there's no reason for using this function in the test either.
In other words, there's no reason to have .curRealmPath, all it does is create surface area for vulnerabilities.
| @@ -36,4 +36,5 @@ var Package = amino.RegisterPackage(amino.NewPackage( | |||
| TooManySignaturesError{}, "TooManySignaturesError", | |||
| NoSignaturesError{}, "NoSignaturesError", | |||
| GasOverflowError{}, "GasOverflowError", | |||
| RestrictedTransferError{}, "RestrictedTransferError", | |||
There was a problem hiding this comment.
and this should also live not in tm2 but gno near GnoAccount
| func (prm *SDKParams) SetBool(key string, value bool) { | ||
| prm.assertRealmAccess() | ||
| realmParamKey := fmt.Sprintf("%s.%s", prm.curRealmPath, lockSendKey) | ||
| if key == realmParamKey { |
There was a problem hiding this comment.
this can't be the right design.
-
curRealmPath itself is weird; it's a variable field of SDKParams, presumably it can change values, even though it's only supposed to be r/sys/params today, it would be easy to suggest a change that removes this restriction. then, this
if key == realmParamKeycondition could succeed where it should not, because another curRealmPath tried to use lockSendKey. -
we're going to end up with all these custom prm.vmk.xxx.Yyy() injections in this file, but they would be grouped by type. I'm not sure exactly how, but really they should be grouped by the realm part of the key, if anything. And adding onto the point 1 above, the realm path should be extracted from the key, and compared against a const, not depend on curRealmPath.
thinking...
There was a problem hiding this comment.
Suggested modification:
func (prm *SDKParams) SetBool(key string, value bool) {
prm.assertRealmAccess(key) // uses the key to assert
kpr := prm.keeperMapper.GetKeeper(key) // keeperMapper defined in https://github.com/gnolang/gno/pull/3176/files#r1908698885
if kpr != nil {
kpr.WillSetParam(key, value) // could be WillSetBool() but WillSetParam() seems simpler.
}
prm.vmk.prmk.SetBool(prm.ctx, key, value)
}Better yet I think is to just merge keeperMapper into SDKParams, and make SDKParams something that takes the context explicitly (like a keeper), and doesn't go through vmk to access needed keepers. See comments on SDKParams for more info.
func (prm *SDKParams) SetBool(ctx sdk.Context, key string, value bool) {
prm.assertRealmAccess(key) // uses the key to assert
kpr := prm.GetRegisteredKeeper(key)
if kpr != nil {
kpr.WillSetParam(ctx, key, value) // could be WillSetBool() but WillSetParam() seems simpler.
}
prm.prmk.SetBool(ctx, key, value)
}
| { | ||
| name: "SetString should panic", | ||
| setFunc: func() { | ||
| params.SetString("name.string", "foo") |
There was a problem hiding this comment.
these tests are misleading.
the expected format is <realm>.<paramname>.<type>.
so if anything, what should be passed in is something like "gno.land/r/foo.name.string".
which also means params.SetCurRealmPath("gno.land/r/foo") is an unnecessary call.
See my other comments about curRealmPath.
| @@ -1,54 +0,0 @@ | |||
| // Package params provides functions for creating parameter executors that | |||
There was a problem hiding this comment.
Ditto, I don't think these should be deleted.
| @@ -1,15 +0,0 @@ | |||
| package params | |||
| } | ||
|
|
||
| func (vm *VMKeeper) SetParams(ctx sdk.Context, params Params) error { | ||
| if params.Equals(Params{}) { |
There was a problem hiding this comment.
I don't see why we'd want this block.
| @@ -19,6 +19,7 @@ type ParamsInterface interface { | |||
| SetInt64(key string, val int64) | |||
| SetUint64(key string, val uint64) | |||
| SetBytes(key string, val []byte) | |||
| SetCurRealmPath(curRealmPath string) | |||
There was a problem hiding this comment.
see other comments about curRealmPath; should be removed.
| _, rlmPath := currentRealm(m) | ||
| GetContext(m).Params.SetCurRealmPath(rlmPath) |
| @@ -131,7 +131,6 @@ func (n *Node) ExportStateAsGenesis(ctx context.Context) (*bft.GenesisDoc, error | |||
|
|
|||
| // Get current blockstore state | |||
| doc := *n.Node.GenesisDoc() // copy doc | |||
|
|
|||
| genState := doc.AppState.(gnoland.GnoGenesisState) | |||
| genState.Balances = n.config.BalancesList | |||
| genState.Txs = state | |||
There was a problem hiding this comment.
just a note that the deleted files below this file should be expanded for additional comments.
| km := params.NewPrefixKeyMapper() | ||
| km.RegisterPrefix(auth.ParamsPrefixKey) | ||
| km.RegisterPrefix(bank.ParamsPrefixKey) | ||
| km.RegisterPrefix(vm.ParamsPrefixKey) |
There was a problem hiding this comment.
How about instead of PrefixKeyMapper, we call it KeeperMapper, and we register not just the prefix key, but we register the keeper itself?
e.g. auth, bank, vm could implement the following interface
type ParamfulKeeper interface {
GetParamsKey() string
}and km.RegisterKeepers(auth, bank, vm).
This way, we can actually do a lookup in Params.SetBool (for example) to look up the keeper first by prefix.
See specific suggestion in https://github.com/gnolang/gno/pull/3176/files#r1908697754 which actually suggests removing PrefixKeyMapper/KeeperMapper entirely and just baking it into SDKParams directly.
| @@ -62,19 +65,59 @@ func (bnk *SDKBanker) RemoveCoin(b32addr crypto.Bech32Address, denom string, amo | |||
| type SDKParams struct { | |||
| vmk *VMKeeper | |||
| ctx sdk.Context | |||
| // The curRealmPath is used to track the current realm accessing the SDKParams from the VM. | |||
| // It serves as a safeguard to control access from the VM. | |||
| curRealmPath string | |||
| } | |||
There was a problem hiding this comment.
I think ctx should be removed, and curRealmPath removed for sure, but also maybe vmk removed too, and replaced with the following:
type SDKParams struct {
pmk *ParamsKeeper
kprs map[prefix]ParamfulKeeper
}And the methods like SetBool(key, string, value bool) take ctx explicitly, much like a keeper itself.
func (prm *SDKParams) SetBool(ctx sdk.Context, key string, value bool) ...By passing in ctx in the methods, there is no need to create new SDKParams instances for each context.
And instead of accessing keepers indirectly via prm.vmk.bank.AddRestrictedDenoms, the bank keeper can just be registered as a ParamfulKeeper in prm directly. One benefit is that keepers that aren't even relevant to the VMKeeper can be registered this way. And besides, if you need to access a private field keeper of bank (e.g. prm.vmk.bank.other.XYZ()), you can't do it due to Go's security model, and that's the whole point of keepers anyways.
| @@ -64,8 +65,9 @@ type VMKeeper struct { | |||
| baseKey store.StoreKey | |||
| iavlKey store.StoreKey | |||
| acck auth.AccountKeeper | |||
| bank bank.BankKeeper | |||
| bank *bank.BankKeeper | |||
There was a problem hiding this comment.
why did this have to change? there's a change in security (keepers are ideally immutable, though if we have mutable fields maybe this is OK, but managing mutable fields in keepers is tricky.
See https://github.com/gnolang/gno/pull/3176/files#r1908750448
| @@ -12,11 +12,11 @@ import ( | |||
| ) | |||
|
|
|||
| type bankHandler struct { | |||
| bank BankKeeper | |||
| bank *BankKeeper | |||
| prefix string | ||
| key store.StoreKey | ||
| // prefix string | ||
| prefixKeyMapper PrefixKeyMapper |
There was a problem hiding this comment.
see my other comments about merging prefixKeyMapper into Params: https://github.com/gnolang/gno/pull/3176/files#r1908697754
| import "fmt" | ||
|
|
||
| // PrefixKeyMapper is used to map one key string to another. | ||
| type PrefixKeyMapper struct { |
There was a problem hiding this comment.
second this. the paramkeeper should sanitize. IsASCIIText() somewhere.
I don't have an opinion on this yet, except to say that it belongs in GnoAccount (and we can in the future move them to BaseAccount if other projects want to use it); and that bitflags kind of mitigate the problem. See comment in code.
This is my primary concern; adding a new param to a keeper would require migration code, unless amino supports appending fields (without changing the order of prior fields). But maybe that is the answer. Otherwise I don't see how per-module param updates can be maintained cleanly, except what Milos mentioned; basically breaking down the per-module param fields into individual values, and storing them separately, yet still having the module maintain the list of parameters, and somehow also managing upgrades/additions/deprecations of per-module param keys.
I think this can be addressed by the ParamfulKeeper and keeperMapper baked directly into SDKParams directly. See prior comments on files for more info, namely https://github.com/gnolang/gno/pull/3176/files#r1908697754, https://github.com/gnolang/gno/pull/3176/files#r1908698885, and https://github.com/gnolang/gno/pull/3176/files#r1908743583. And check the comments on deleted files; even if we support this per-module typed param system, we still want to support the previous system, so the original tests should not be deleted. For one, if we're going to base SDKParam on the ParamKeeper, it makes sense for ParamKeeper to have good tests independet of SDKParam. But also, I can still imagine keepers and non-keepers needing params that are more free-form; for example, parameters for a specific gno realm, such as per-account information, stored in the ParamKeeper with a key like /vm/realmparams/gno.land/r/something..Username = "Bob". The reason for using such a system might be that this allows for fast per-account username lookup without needing to access the Gno value tree or VM, but rather just an AVL tree lookup (i.e. optimization).
I think this is worth considering too before merging. |
Context for Locking Token Transfer
This feature allows us to lock token transfers, except for paying gas fees to add a package or call a contract. The restriction will be unlocked through GovDAO voting on a proposal.
We also want a few whitelisted, unrestricted accounts to vote on the proposal since a separate fee is required to initiate and vote on proposals.
This implementation manages the lock and unlock settings in r/sys/params, where we change the chain’s parameters. Calling lock or unlock will automatically submit a proposal to GovDAO. Once GovDAO votes and approves the proposal, the token
transfer restriction will be removed instantly.
All changes to parameters specified in r/sys/params must go through GovDAO voting.
Here are some implementation details
Main Idea Behind the Alternative Approach To implement parameter module ( Discussion)
Todo: update other params...
Contributors' checklist...